test: adding big block tests to the main branch #3612
Conversation
|
I re-ran a subset of benchmark tests, and they looked good, hence opening this PR for review. |
|
The CI failures will be resolved by #3653 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| logger.Println("=== RUN TwoNodeSimple", "version:", latestVersion) | ||
| testName := "TwoNodeSimple" | ||
| logger.Printf("Running %s\n", testName) | ||
| logger.Println("version", latestVersion) |
There was a problem hiding this comment.
is this missing a formatting directive?
| logger.Println("version", latestVersion) | |
| logger.Printf("version %v\n", latestVersion) |
There was a problem hiding this comment.
They both indeed work identically:
The Println function prints the supplied inputs with comma separation, whereas in Printf, I achieved the same result using formatting. Here is a sample output from the tests:
test-e2e-benchmark2024/07/08 11:34:05 Running TwoNodeSimple
test-e2e-benchmark2024/07/08 11:34:05 version 8caa580
Their implementation is just inconsistent, which I can fix in a follow-up PR.
| if size > maxBlockSize { | ||
| maxBlockSize = size | ||
| } |
There was a problem hiding this comment.
nit: I don't think these lines will be executed if the previous conditional evaluates to true b/c the break so consider re-ordering this above the previous conditional so that maxBlockSize accounts for the block that actually exceeds >= expectedBlockSizeBytes
There was a problem hiding this comment.
The current version is correct as well. I see what causes confusion here: the value of maxBlockSize is only used when the target size is not reached. That is, if size >= expectedBlockSizeBytes is never hit, and if that condition is never met, then maxBlockSize correctly reflects the most recent maximum size observed. If we do hit the expectedBlockSizeBytes condition and if size >= expectedBlockSizeBytes evaluates to true, then the value of maxBlockSize does not matter. Nevertheless, to resolve future confusion, I will apply your suggestion in a follow up PR, thanks!
|
The markdown linter complaint is unrelated to this PR and will be resolved independently in #3668 |
Part of #3557
Closes #3614
Closes #3638
Closes #3371